test(unit): add comprehensive unit test suites for core library classes & lib#1696
test(unit): add comprehensive unit test suites for core library classes & lib#1696Shadow243 wants to merge 7 commits intocypht-org:masterfrom
Conversation
2db6857 to
856f038
Compare
6ec3c46 to
ac583ca
Compare
tests/phpunit/lib/searchable.php
Outdated
|
|
||
| public function setUp(): void { | ||
| if (!defined('APP_PATH')) { | ||
| require_once __DIR__.'/../bootstrap.php'; |
There was a problem hiding this comment.
Proper bootstrap means we don't call these inside the unit tests themselves. Why would the app path not be defined? Tests should be executed by a single phpunit command which should always properly bootstrap the framework.
tests/phpunit/lib/searchable.php
Outdated
| } | ||
| // Load mocks first | ||
| if (!class_exists('Hm_Mock_Session', false)) { | ||
| require_once APP_PATH.'tests/phpunit/mocks.php'; |
There was a problem hiding this comment.
This is not the correct place to include such files. Do it in the bootstrap system.
tests/phpunit/mocks.php
Outdated
| } | ||
| } | ||
|
|
||
| trait Mock_Searchable { |
There was a problem hiding this comment.
None of the classes or traits called mocks are actual mocks - you copy/implement details from the actual code in the test suite - this is counterproductive as code will diverge over time. See the documentation on how to mock objects https://docs.phpunit.de/en/9.6/test-doubles.html#mock-objects - you add some expectations to methods that should be called and what should be returned but you actually work with the real objects and not some classes that resemble the real classes partially. In theory, it is best to follow this pattern:
- use the object itself whenever possible
- if testing becomes complicated as the original class depends on 3rd party external connections, you can mock the result of some of its methods and return what you want to test
- note that merely testing the return of data that you supplied as a mock is not a productive test. The test objective is to test code that runs in the framework and not code that is written in the tests themselves. So, whenever you want to test a method, see what external connections it uses, mock them and actually call that method on that original class/object and test that it works correctly. There is a subtle but important difference here. The way you did it with these mocks is that you test the mocks themselves which doesn't make sense.
Also, you shouldn't use reflection to set private properties or do similar changes that were not designed to be done with the class in question. If you want to test something that's not available for some reason, do:
- think if it actually needs to be tested. Private methods usually are not tested. Testing the public interface of the class is enough to ensure it works fine as it calls private methods internally, so they become part of the tests.
- if something private really needs to be tested, refactor the original class/interface and make it public or separate it to parts that are testable
Refactoring original code because of testing is a legitimate use-case, so don't be afraid to do it.
There was a problem hiding this comment.
@kroky I’ve applied all the requested corrections and fixed the other failing tests.
For Hm_Test_Mailbox, I added some imports in setUp to avoid conflicts. These are only required there and not elsewhere.
There was a problem hiding this comment.
In addition, in hm-jmap.php I added a few missing compatibility functions (is_supported, get_message_sort_order, and sort_by_fetch) with TODOs and comments. These provide placeholders so we can later implement proper JMAP behavior and confirm everything works correctly with JMAP as well.
ac583ca to
d4516b0
Compare
4415838 to
8970896
Compare
…arch content tests
0a4b43f to
896b622
Compare
modules/imap/hm-ews.php
Outdated
| protected $ews; | ||
| protected $api; | ||
| protected $authed = false; | ||
| if (!class_exists('Hm_EWS')) { |
There was a problem hiding this comment.
Please remove this check. Require the file just ones and it will be good. You changed the whole file indentation because of this check when PHP already has a mechanism for including class files only once.
896b622 to
72ddbea
Compare
|
This is a very big pull request and affects too many modules. A more iterative approach (closing this and submitting smaller) module-specific PRs would likely lead to faster feedback and safer merges. What's your thoughts @kroky @Shadow243 ? |
|
I prefer we do all the related/intended work on the tests here in one PR even if it becomes big. It is easier to maintain from my point of view. |
New Test Suites
tests/phpunit/modules/core/mailbox.php – Tests for Hm_Mailbox
tests/phpunit/lib/searchable.php – Tests for Searchable trait
tests/phpunit/lib/scram_authenticator.php – SCRAM authentication
tests/phpunit/lib/ini_set.php – PHP configuration validation